ACP implementation bringing 31 CLI tools into t3code#2684
Open
LivioGama wants to merge 1 commit into
Open
Conversation
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
github-actions
Bot
added
vouch:unvouched
![macroscopeapp[bot]](https://avatars.githubusercontent.com/in/900172?s=60&v=4){kind=small}
LivioGama
force-pushed
the
feat/acp-registry
branch
from
085f518 to
ee86ab9
Compare
LivioGama
force-pushed
the
feat/acp-registry
branch
12 times, most recently
from
9126d46 to
8c96e1e
Compare
LivioGama
force-pushed
the
feat/acp-registry
branch
2 times, most recently
from
c5c0bb4 to
3e1f7f9
Compare
LivioGama
force-pushed
the
feat/acp-registry
branch
2 times, most recently
from
74107bf to
43441d2
Compare
LivioGama
force-pushed
the
feat/acp-registry
branch
from
43441d2 to
cd5e45c
Compare
Contributor
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
Adds first-class support for bundling, installing, and running ACP Registry agents across contracts, server, and web UI, including install state persistence and authentication UX.
Changes:
- Introduces ACP registry schemas/contracts, bundled registry snapshot access, and new WS/IPC RPCs for listing/install/uninstall/authenticate.
- Implements server-side ACP registry install pipeline (binary/npx/uvx), install manifest migration/persistence, and a generic
AcpRegistryDriverper bundled agent. - Updates web UI to surface registry agents inside Providers, add inline model picker UX, and render ACP agent icons.
Reviewed changes
Copilot reviewed 66 out of 101 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| scripts/sync-acp-registry.ts | Adds a script to sync the upstream ACP registry snapshot and icons into contracts. |
| packages/shared/src/serverSettings.ts | Ensures whole-map replacement semantics for acpRegistryInstalls patches. |
| packages/shared/src/serverSettings.test.ts | Adds regression test for replacing acpRegistryInstalls entries. |
| packages/effect-acp/src/client.ts | Adjusts JSON-RPC request id generation for JVM agent compatibility. |
| packages/contracts/src/settings.ts | Adds ACP registry settings schema and server settings fields for install state. |
| packages/contracts/src/server.ts | Extends server provider auth schema with ACP-advertised auth methods. |
| packages/contracts/src/rpc.ts | Adds ACP registry WS RPC method contracts and wires them into the RPC group. |
| packages/contracts/src/registry/index.ts | Exposes the bundled registry document and lookup helpers. |
| packages/contracts/src/registry/icons/vtcode.svg | Adds ACP registry agent icon asset. |
| packages/contracts/src/registry/icons/stakpak.svg | Adds ACP registry agent icon asset. |
| packages/contracts/src/registry/icons/sigit.svg | Adds ACP registry agent icon asset. |
| packages/contracts/src/registry/icons/qwen-code.svg | Adds ACP registry agent icon asset. |
| packages/contracts/src/registry/icons/qoder.svg | Adds ACP registry agent icon asset. |
| packages/contracts/src/registry/icons/poolside.svg | Adds ACP registry agent icon asset. |
| packages/contracts/src/registry/icons/pi-acp.svg | Adds ACP registry agent icon asset. |
| packages/contracts/src/registry/icons/opencode.svg | Adds ACP registry agent icon asset. |
| packages/contracts/src/registry/icons/nova.svg | Adds ACP registry agent icon asset. |
| packages/contracts/src/registry/icons/mistral-vibe.svg | Adds ACP registry agent icon asset. |
| packages/contracts/src/registry/icons/minion-code.svg | Adds ACP registry agent icon asset. |
| packages/contracts/src/registry/icons/kimi.svg | Adds ACP registry agent icon asset. |
| packages/contracts/src/registry/icons/kilo.svg | Adds ACP registry agent icon asset. |
| packages/contracts/src/registry/icons/junie.svg | Adds ACP registry agent icon asset. |
| packages/contracts/src/registry/icons/goose.svg | Adds ACP registry agent icon asset. |
| packages/contracts/src/registry/icons/glm-acp-agent.svg | Adds ACP registry agent icon asset. |
| packages/contracts/src/registry/icons/github-copilot-cli.svg | Adds ACP registry agent icon asset. |
| packages/contracts/src/registry/icons/gemini.svg | Adds ACP registry agent icon asset. |
| packages/contracts/src/registry/icons/factory-droid.svg | Adds ACP registry agent icon asset. |
| packages/contracts/src/registry/icons/dirac.svg | Adds ACP registry agent icon asset. |
| packages/contracts/src/registry/icons/dimcode.svg | Adds ACP registry agent icon asset. |
| packages/contracts/src/registry/icons/deepagents.svg | Adds ACP registry agent icon asset. |
| packages/contracts/src/registry/icons/cursor.svg | Adds ACP registry agent icon asset. |
| packages/contracts/src/registry/icons/crow-cli.svg | Adds ACP registry agent icon asset. |
| packages/contracts/src/registry/icons/corust-agent.svg | Adds ACP registry agent icon asset. |
| packages/contracts/src/registry/icons/cortex-code.svg | Adds ACP registry agent icon asset. |
| packages/contracts/src/registry/icons/codex-acp.svg | Adds ACP registry agent icon asset. |
| packages/contracts/src/registry/icons/codebuddy-code.svg | Adds ACP registry agent icon asset. |
| packages/contracts/src/registry/icons/cline.svg | Adds ACP registry agent icon asset. |
| packages/contracts/src/registry/icons/claude-acp.svg | Adds ACP registry agent icon asset. |
| packages/contracts/src/registry/icons/autohand.svg | Adds ACP registry agent icon asset. |
| packages/contracts/src/registry/icons/auggie.svg | Adds ACP registry agent icon asset. |
| packages/contracts/src/registry/icons/amp-acp.svg | Adds ACP registry agent icon asset. |
| packages/contracts/src/registry/icons/agoragentic-acp.svg | Adds ACP registry agent icon asset. |
| packages/contracts/src/providerInstance.ts | Adds authenticatedAt field to provider instance config. |
| packages/contracts/src/ipc.ts | Adds Local API contract surface for ACP registry operations. |
| packages/contracts/src/index.ts | Re-exports ACP registry contracts and bundled registry entrypoints. |
| packages/contracts/src/acpRegistry.ts | Introduces ACP registry schemas, install state, and helper utilities. |
| package.json | Adds sync:acp-registry script. |
| docs/providers/acp-registry.md | Documents ACP registry bundling, install pipeline, and UI surfaces. |
| apps/web/vite.config.ts | Adds Vite plugin to copy ACP icons into the web public directory at build/start. |
| apps/web/src/rpc/wsRpcClient.ts | Adds ACP registry client methods to the WS RPC client. |
| apps/web/src/routes/settings.acp-registry.tsx | Adds legacy route redirect to Providers settings. |
| apps/web/src/routeTree.gen.ts | Registers the new ACP registry settings route in the generated router tree. |
| apps/web/src/modelSelection.test.ts | Updates test fixtures to include enabled in provider instances. |
| apps/web/src/localApi.ts | Exposes ACP registry methods via the browser Local API wrapper. |
| apps/web/src/components/settings/SettingsPanels.tsx | Merges ACP registry into Providers panel and updates model picker UX. |
| apps/web/src/components/settings/ProviderInstanceCard.tsx | Adds ACP auth UI for registry-backed provider instances. |
| apps/web/src/components/chat/ProviderStatusBanner.tsx | Shows a friendly auth-required banner for unauthenticated ACP agents. |
| apps/web/src/components/chat/ProviderModelPicker.tsx | Refactors model picker trigger to coordinate with new inline panel. |
| apps/web/src/components/chat/ProviderModelPicker.browser.tsx | Updates browser tests to mount trigger + inline picker content together. |
| apps/web/src/components/chat/ProviderInstanceIcon.tsx | Adds rendering of ACP registry icons for registry-backed drivers. |
| apps/web/src/components/chat/ModelPickerSidebar.tsx | Switches provider picker to inline tile row and groups ACP entries. |
| apps/web/src/components/chat/ChatComposer.tsx | Adds inline model picker panel above the composer when open. |
| apps/web/src/components/AcpRegistryIcon.tsx | Adds a component to render ACP icons (mask-based) with fallback initials. |
| apps/server/src/ws.ts | Wires ACP registry RPC handlers and provides the registry service layer. |
| apps/server/src/textGeneration/AcpRegistryTextGeneration.ts | Adds unsupported text-generation shim for ACP registry agents. |
| apps/server/src/serverSettings.test.ts | Updates tests to include enabled in provider instance configs. |
| apps/server/src/server.test.ts | Mocks ACP registry service for server tests. |
| apps/server/src/provider/builtInDrivers.ts | Registers one generic driver per bundled ACP registry entry. |
| apps/server/src/provider/acp/configOptionModels.ts | Builds model lists from ACP session setup/config options. |
| apps/server/src/provider/acp/AcpSessionRuntime.ts | Makes auth optional and captures auth methods + available modes. |
| apps/server/src/provider/acp/AcpMultiSession.ts | Introduces multi-session ACP runtime with event parsing and config/mode setters. |
| apps/server/src/provider/acp/AcpAdapterSupport.ts | Adds helper to resolve ACP permission outcomes to agent-advertised option ids. |
| apps/server/src/provider/acp/AcpAdapterSupport.test.ts | Adds tests for permission outcome resolution. |
| apps/server/src/provider/Services/AcpRegistryAdapter.ts | Adds adapter shape for ACP registry provider integration. |
| apps/server/src/provider/Layers/acpRegistryAdapter/types.ts | Adds types for ACP registry adapter session and event context. |
| apps/server/src/provider/Layers/acpRegistryAdapter/permissionHandlers.ts | Implements permission request handling + decision bridging. |
| apps/server/src/provider/Layers/acpRegistryAdapter/helpers.ts | Adds helper to reconcile previously-selected ACP model against agent options. |
| apps/server/src/provider/Layers/acpRegistryAdapter/fileHandlers.ts | Adds secure file read/write handlers for ACP sessions (cwd sandboxing). |
| apps/server/src/provider/Layers/acpRegistryAdapter/fileHandlers.test.ts | Adds tests for ACP path sandboxing. |
| apps/server/src/provider/Layers/acpRegistryAdapter/eventForwarding.ts | Forwards ACP session events into provider runtime events. |
| apps/server/src/provider/Layers/ProviderInstanceRegistryHydration.ts | Ensures hydrated legacy provider instances default to enabled. |
| apps/server/src/provider/Drivers/AcpRegistryDriver.ts | Implements data-driven provider driver for each ACP registry agent. |
| apps/server/src/provider/Drivers/AcpRegistryDriver.test.ts | Adds tests for ACP model option parsing. |
| apps/server/src/config.ts | Adds derived cache dir for ACP registry installs and ensures it exists. |
| apps/server/src/acpRegistry/platform.ts | Maps Node platform/arch to ACP registry binary platform literals. |
| apps/server/src/acpRegistry/orphanReaper.ts | Adds boot-time orphan process reaping for detached ACP agents. |
| apps/server/src/acpRegistry/installer.ts | Implements install/uninstall and safe extraction/spawn-target resolution. |
| apps/server/src/acpRegistry/installer.test.ts | Adds installer tests for binary installs, checksums, and path safety. |
| apps/server/src/acpRegistry/installManifest.ts | Adds manifest persistence + migration from settings with atomic writes. |
| apps/server/src/acpRegistry/installManifest.test.ts | Adds tests for manifest read/write, migration, and corruption handling. |
| apps/server/src/acpRegistry/childProcessRegistry.ts | Adds process-wide child PID tracking for hard shutdown cleanup. |
| apps/server/src/acpRegistry/AcpRegistryService.test.ts | Adds tests for auth probe timeouts by distribution. |
| AGENTS.md | Documents ACP registry support and points to the new provider docs. |
Comments suppressed due to low confidence (4)
apps/web/vite.config.ts:1
__dirnameis not defined in ESM (which Vite config commonly runs as). This will throw at runtime and prevent the dev server/build from starting. UsefileURLToPath(import.meta.url)+path.dirname(...)(or Vite'snew URL(..., import.meta.url)) instead of__dirnameto resolve paths.
apps/web/src/components/AcpRegistryIcon.tsx:1- This element sets both
aria-labelandaria-hidden, which is contradictory (screen readers will ignore the label). Decide whether the icon is decorative (keeparia-hiddenand removerole/aria-label) or meaningful (removearia-hiddenand keep an appropriate label).
scripts/sync-acp-registry.ts:1 - The script currently excludes no agents, but the docs and surrounding changes indicate overlapping first-party drivers (e.g.
claude-acp,cursor,opencode,codex-acp) should be filtered out of the bundled snapshot. PopulateEXCLUDED_AGENT_IDSwith the intended ids (or load them from config) so the generatedregistry.jsonmatches the documented behavior.
docs/providers/acp-registry.md:1 - This description doesn't match the implementation in the PR: the sync script only writes into the contracts
icons/directory, while the webpublic/acp-iconspopulation is handled by a Vite plugin at build/start time. Update this doc to reflect the actual pipeline (and/or update the script to copy intoapps/web/public/acp-icons/as described) to avoid stale operational instructions.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+553
to
+560
| install: (agentId) => | ||
| Effect.fail( | ||
| new AcpRegistryError({ | ||
| agentId, | ||
| detail: "ACP registry install is disabled in tests.", | ||
| }), | ||
| ), | ||
| uninstall: () => Effect.void, |
Comment on lines
+100
to
+110
| // Cleanup: clear stale settings.json entries after successful migration | ||
| yield* settingsService | ||
| .updateSettings({ acpRegistryInstalls: {} }) | ||
| .pipe( | ||
| Effect.asVoid, | ||
| Effect.mapError( | ||
| (cause) => | ||
| new InstallManifestError({ detail: "Failed to cleanup stale settings after migration", cause }), | ||
| ), | ||
| ); | ||
| yield* Effect.logInfo("Migrated ACP registry installs from settings.json to manifest and cleaned up stale entries"); |
Comment on lines
+152
to
+174
| it("uses PowerShell Expand-Archive on Windows for zip extraction", async () => { | ||
| // This test verifies the Windows extraction path is correctly configured | ||
| // Actual extraction is platform-specific, so we only test the path resolution | ||
| const cacheRoot = await fs.mkdtemp(path.join(os.tmpdir(), "t3-acp-install-")); | ||
| const entry = rawEntry("", "./agent.exe"); | ||
|
|
||
| // Mock Windows platform | ||
| const originalPlatform = process.platform; | ||
| Object.defineProperty(process, "platform", { value: "win32" }); | ||
|
|
||
| try { | ||
| // The installer should handle Windows-specific extraction | ||
| // We can't actually test PowerShell execution in cross-platform tests, | ||
| // but we can verify the code path exists and is reachable | ||
| expect( | ||
| (entry.distribution.binary as Record<string, unknown> | undefined)?.["win32-x86_64"], | ||
| ).toBeUndefined(); | ||
| // This is a minimal test to ensure the Windows path is covered | ||
| // Full extraction testing would require Windows-specific CI | ||
| } finally { | ||
| Object.defineProperty(process, "platform", { value: originalPlatform }); | ||
| await fs.rm(cacheRoot, { recursive: true, force: true }); | ||
| } |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 2 potential issues.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit cd5e45c. Configure here.
| }; | ||
| connections.set(key, pooled); | ||
| return pooled; | ||
| }); |
Contributor
There was a problem hiding this comment.
Connection pool race condition leaks child processes
Medium Severity
The connections Map is a plain mutable Map accessed by concurrent fibers without synchronization. In acquireConnection, between connections.get(key) returning undefined (line 201) and connections.set(key, pooled) (line 233), there are multiple yield* async boundaries where other fibers can run. Two concurrent callers (e.g., discoverModels fork and a startSession call) targeting the same key can both see no existing entry, both spawn a child process, and the second set overwrites the first — permanently leaking the first connection's child process.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit cd5e45c. Configure here.
| spawnTarget.args.join("\0"), | ||
| cwd, | ||
| envFingerprint, | ||
| ].join(""); |
Contributor
There was a problem hiding this comment.
Connection pool key uses ambiguous empty-string join
Low Severity
connectionKey joins the four key parts (command, args, cwd, envFingerprint) with an empty string via .join(""). Without delimiters, boundaries between parts are ambiguous — different (command, args, cwd) tuples can produce identical keys. For example, command "npx" with args ["agent/work"] and cwd "" produces the same key as command "npx" with args ["agent"] and cwd "/work" (assuming matching env). This could cause distinct spawn configurations to incorrectly share a pooled connection.
Reviewed by Cursor Bugbot for commit cd5e45c. Configure here.
Contributor
ApprovabilityVerdict: Needs human review Diff is too large for automated approval analysis. A human reviewer should evaluate this PR. You can customize Macroscope's approvability policy. Learn more. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Demo
Note
High Risk
High risk because it introduces a new provider execution pipeline that downloads/extracts binaries, spawns/manages child processes, and adds new websocket RPCs/state persistence; mistakes could impact security (path traversal/process control) or stability (orphaned processes, timeouts, caching).
Overview
Adds an ACP Registry provider system end-to-end. The server now ships a bundled ACP registry snapshot and exposes new WS RPCs to
list,install,uninstall, andauthenticateregistry agents, including automatic provider-instance creation/backfill.Implements installation + persistence for registry agents. Adds a hardened installer supporting
binary/npx/uvxdistributions with archive extraction, path validation, optional SHA256 verification, and a new on-disk install manifest (acp-agents/installs.json) that migrates legacy settings and cleans up stale entries.Introduces a new ACP runtime stack and reliability safeguards. Adds
AcpConnection+AcpMultiSessionwith connection pooling, model discovery/caching (with boot-time warm discovery + failure backoff), spec fixes (optionalcategory, permission option resolution), and multi-layer child-process reaping to prevent orphaned agents; web composer UI is updated to use an inline model picker layout and addsAcpRegistryIconrendering.Reviewed by Cursor Bugbot for commit cd5e45c. Bugbot is set up for automated code reviews on this repo. Configure here.
Note
Add ACP registry support with 31 CLI tools, install/uninstall flows, and WebSocket RPC endpoints
packages/contracts/src/registry/registry.json) with 31 agents, schema-validated types, and async:acp-registryscript to refresh the snapshot from upstream.AcpRegistryServicewithlist,install,uninstall, andauthenticateoperations; binary agents are downloaded and cached underacpRegistryCacheDir, while npx/uvx agents use package invocation.acpRegistryList/Install/Uninstall/Authenticate) wired throughmakeWsRpcLayerand proxied through the local browser API.AcpRegistryDriverthat creates provider driver instances for each bundled agent, reaps orphan processes on startup, and surfaces discovered models from session config options.InlineProviderTileRowgrouping built-in and ACP registry providers, and movesProviderModelPickerto a trigger-only button that opens an inline panel.AddOrInstallProviderPanelin settings for browsing, installing, and configuring ACP agents inline, replacing the previousAddProviderInstanceDialog.applyServerSettingsPatchnow replacesacpRegistryInstallswholesale instead of deep-merging, so patch omissions delete existing keys.reapOrphanProcesses) sends SIGKILL to processes matching a binary path prefix on Unix; misidentified processes could be killed.Macroscope summarized cd5e45c.